Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix config options for Android release builds #295

Merged

Conversation

wwwdata
Copy link
Contributor

@wwwdata wwwdata commented Feb 24, 2020

Connection with issue(s)

Resolve issue #294

Testing and Review Notes

Using reflection was not a good idea because when the Android App is compiled for release mode, some code optimizations are executed. The fields of the configuration classes were then renamed and the mapping via reflections did not work anymore. Now everything has an explicit mapping from the untyped HashMap to the class fields and vice versa.

In order to test this, just compile a release build of the example repository and try to set some other default options. It will not work. A good example is to set useShouldOverrideUrlLoading to true and then use the callback for it.

@pixelgroup-israel
Copy link

13 days to review this very important change?
Can this be released ASAP?
It voids any release build.

@wwwdata
Copy link
Contributor Author

wwwdata commented Mar 9, 2020

looks like @pichillilorenzo is inactive since December. I hope he wakes up again and maintains this awesome Plugin. But in the meantime, I guess you will have to make your own fork and merge in the patches that you need. This is also how we currently do it for the project I am working for.

Using reflection was not a good idea because when the Android App is
compiled for release mode, some code optimizations are executed. The
fields of the configuration classes were then renamed and the mapping
via reflections did not work again. Now everything has an explicit
mapping form the untyped HashMap to the class fields and vice versa.
@wwwdata wwwdata force-pushed the fix-android-config-options branch from 80731dd to a7100ae Compare March 9, 2020 12:49
@DmitriyIgnashchenko
Copy link

it works, thank you

@BertrandBev
Copy link

Yep, thanks a lot!

@arnaudelub
Copy link

oh, nice, i've opened an issue on this 2 weeks ago: #315

Apparently, another workaround to fix this is to find the missing classes with logcat and add them to proguard.

Maybe we should retake this package and publish a new one on pub.dev. Anyone want to collaborate on this? it's a shame this package is not maintained anymore

@wwwdata
Copy link
Contributor Author

wwwdata commented Apr 16, 2020

@arnaudelub hm, really looks like the original maintainer abandoned this repo here. If you want to make a fork, I would be in to collaborate on it. On my current project, we are heavily using web views and I already have some other minor changes that are internal for now. It would be nice to have a more responsive open source repo where I then could open PRs for everything that I find is missing/needs fixing.

@arnaudelub
Copy link

@wwwdata nice, so let's do it, but i think i'll fork it and rebuild a new plugin so we can put it on pub.dev

@pichillilorenzo
Copy link
Owner

Released new version 3.0.0. Thanks!

This was referenced Jul 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants